Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Finish fixing #17 #78

Merged
merged 15 commits into from
Mar 27, 2020
Merged

Finish fixing #17 #78

merged 15 commits into from
Mar 27, 2020

Conversation

GarkGarcia
Copy link
Contributor

@GarkGarcia GarkGarcia commented Mar 25, 2020

This is a follow up from #17.

I messed something up while trying to pull some changes from upstream, so there are 23 commits from my previous contribution in here.

The only relevants commits are the last two.


This change is Reviewable

@jonhoo
Copy link
Owner

jonhoo commented Mar 25, 2020

Ah, yes, that's my fault because I ended up doing some squashing. If only the last two matter, run:

$ git remote add upstream https://github.com/jonhoo/flurry.git
$ git fetch upstream
$ git rebase --onto upstream/master HEAD~2

That should, in theory, replay just the last two commits onto the current master branch. Then you'll have to do:

$ git push --force-with-lease

To overwrite your current PR branch with the rebased one.

BTW, if you want to learn more about this sort of magic, I highly recommend this lecture.

The documentation hasn't beeng updated yet.
@jonhoo
Copy link
Owner

jonhoo commented Mar 25, 2020

@GarkGarcia I just pushed a commit to master that re-aligns the methods in map_ref.rs with those in map.rs to make comparison easier. We should probably do the same here for set.rs. I've also added a newline above each "See also ..." line, so that it doesn't get lost in the surrounding text in the docs.

src/set_ref.rs Outdated Show resolved Hide resolved
src/set.rs Outdated Show resolved Hide resolved
src/set.rs Outdated Show resolved Hide resolved
src/set.rs Outdated Show resolved Hide resolved
src/set.rs Outdated Show resolved Hide resolved
@GarkGarcia
Copy link
Contributor Author

GarkGarcia commented Mar 26, 2020

We should probably think about implementing HashSet::intersection, HashSet::union and HashSet::difference equivalents.

src/set.rs Outdated Show resolved Hide resolved
src/set_ref.rs Outdated Show resolved Hide resolved
@jonhoo
Copy link
Owner

jonhoo commented Mar 26, 2020

@GarkGarcia Let's leave those for later so that we do not overload this PR too much.

@GarkGarcia
Copy link
Contributor Author

@GarkGarcia Let's leave those for later so that we do not overload this PR too much.

Fair enough.

…hSet::is_superset` take two different guards (one for `self` and another one for `other`)
src/set.rs Outdated Show resolved Hide resolved
@jonhoo
Copy link
Owner

jonhoo commented Mar 27, 2020

Okay, so, apparently I can't push directly to your branch, but I pushed the added commits to pr-17-augment. You should be able to merge my commits using:

$ git fetch upstream
$ git merge upstream/pr-17-augment

Copy link
Owner

@jonhoo jonhoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@jonhoo
Copy link
Owner

jonhoo commented Mar 27, 2020

@GarkGarcia In a follow-up PR, it'd be great to also add a test suite for the ref version of set (just like we have one for map)

@jonhoo jonhoo merged commit d1f67c2 into jonhoo:master Mar 27, 2020
@GarkGarcia
Copy link
Contributor Author

@GarkGarcia In a follow-up PR, it'd be great to also add a test suite for the ref version of set (just like we have one for map)

Makes sense. We could probably include the tests in a PR for #20?

@jonhoo jonhoo mentioned this pull request Mar 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants